-
Notifications
You must be signed in to change notification settings - Fork 603
[Bug] [DAC] Auto Gen Schema Fails on Certain Subqueries #5256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[Bug] [DAC] Auto Gen Schema Fails on Certain Subqueries #5256
Conversation
Bug - GuidelinesThese guidelines serve as a reminder set of considerations when addressing a bug in the code. Documentation and Context
Code Standards and Practices
Testing
Additional Checks
|
…s' of https://github.com/elastic/detection-rules into 5255-bug-dac-auto-gen-schema-fails-on-certain-subqueries
|
This PR also fixes an issue where we did not allow rules with a risk_score of The diff: 9aedd6d is required to update the rule prompt to allow for Additionally, now that we have fixed our schema validation on field types there were typos in To test the risk_score fix/update to allow Outcome: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Executed test-remote-cli from the modified ndjson and works as expected.
Pull Request
Issue link(s):
Resolves #5255
Summary - What I changed
Initial Approach
Added an alignment check for subqueries. This column for error messages can become misaligned with unknown field types in subqueries as the error position may be including an offset in the printed return to show indentation of the subquery instead of its exact location as the function expects. This is caught and handled by checking to see if the character before the expected start of the field name to add is alpha numeric or a
.which is an appropriate field character. If so, then this is not the beginning of the field, and the start should be moved backward until a non field character is reached (e.g. a space, parenthesis, etc.)Since we now parse the EQL sequence queries into sub queries in the new schema validation the original row/column error combination is no longer accurate since this row/column references the original query, not the sub query. My first fix was to have the auto add calculate the correct offset with a max number of retries. But in my testing I found some other situations where it would be impossible to re-calculate the offset. So the better/more correct fix is to parse the row/column when the error is thrown and then pass the field down to the auto add which is what is being done now.
There are also 3 additional fixes in this PR:
This PR also fixes an issue where we did not allow rules with a risk_score of
0which are allowed in Kibana. 373877cThe rule prompt would not accruately load in values of
0which are permitted for other fields than risk_score.The diff: 9aedd6d is required to update the rule prompt to allow for
0values to not be evaluated as False. This patch is required regardless of the risk_score change, as0should be considered a valid input value in the prompt.Additionally, now that we have fixed our schema validation on field types there were typos in
detection_rules/etc/custom-consolidated-rules.ndjsonthe fixed.How To Test
Export a rule with a similar query to the following and an empty
etc/schemas/auto_gen.jsonfile.The file should be appropriately updated with
integration.Fieldinstead ofntegration.Field.Use the following rule as an example:
rules_export_py_fail.ndjson.txt
And run:
This should result in the following rule. One can also run view rule on the following rule instead to achieve the same effect.
Details
We also need to support validation and auto gen schema for rules in this case (the alternate EQL Sequence case)
Details
To test the risk_score fix/update to allow
0:Outcome:
make_test_cli.txt
Checklist
bug,enhancement,schema,maintenance,Rule: New,Rule: Deprecation,Rule: Tuning,Hunt: New, orHunt: Tuningso guidelines can be generatedmeta:rapid-mergelabel if planning to merge within 24 hoursContributor checklist